Skip to content

PHPLIB-1597 Accept a $-prefixed string anywhere an expression is accepted#1571

Merged
GromNaN merged 3 commits intomongodb:v1.xfrom
GromNaN:PHPLIB-1597
Feb 10, 2025
Merged

PHPLIB-1597 Accept a $-prefixed string anywhere an expression is accepted#1571
GromNaN merged 3 commits intomongodb:v1.xfrom
GromNaN:PHPLIB-1597

Conversation

@GromNaN
Copy link
Member

@GromNaN GromNaN commented Jan 23, 2025

Fix PHPLIB-1597

The string representation of a field path is simpler than using the FieldPath factory.

Before:

            Stage::group(
                _id: Expression::fieldPath('sizes'),
                averagePrice: Accumulator::avg(
                    Expression::numberFieldPath('price'),
                ),
            ),

After:

            Stage::group(
                _id: '$sizes'
                averagePrice: Accumulator::avg('$price'),
            ),

@GromNaN GromNaN requested a review from a team as a code owner January 23, 2025 14:05
@GromNaN GromNaN requested a review from jmikola January 23, 2025 14:05
jmikola
jmikola reviewed Jan 23, 2025
View reviewed changes
generator/config/expressions.php Outdated
foreach ($bsonTypes as $name => $acceptedTypes) {
// an expression can be a string with a $-prefixed field name
if (! in_array('string', $acceptedTypes)) {
$acceptedTypes[] = 'string';
Copy link
Contributor

@jmikola jmikola Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is the only code change in this PR, what about this change allows $-prefixed strings specifically? It looks like you're just accepting any kind of string whenever an expression is expected.

Side note: I couldn't confirm if this was really the only code change in the PR because GitHub's PR view is not correctly labeling all generated files. In the future it'd be preferable to segregate changes to generated files into their own commit.

Copy link
Member Author

@GromNaN GromNaN Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added validation of $string, but that's something the server would do.

Failed to optimize pipeline :: caused by :: $abs only supports numeric types, not string

@GromNaN GromNaN force-pushed the PHPLIB-1597 branch 2 times, most recently from bbe5e26 to 9f5a3fb Compare January 24, 2025 11:35
@GromNaN
Copy link
Member Author

GromNaN commented Jan 27, 2025

I had to change the order of conditions to please psalm. https://psalm.dev/r/4e4cd7b846

@GromNaN GromNaN enabled auto-merge (squash) January 27, 2025 14:19
@GromNaN GromNaN force-pushed the PHPLIB-1597 branch 2 times, most recently from b7c1dc6 to 8dc642c Compare January 28, 2025 17:17
@GromNaN GromNaN disabled auto-merge January 28, 2025 17:34
@GromNaN GromNaN force-pushed the PHPLIB-1597 branch 2 times, most recently from 048454e to 663418d Compare January 29, 2025 15:06
@GromNaN GromNaN enabled auto-merge (squash) January 29, 2025 15:08
@GromNaN GromNaN merged commit e352fb3 into mongodb:v1.x Feb 10, 2025
30 checks passed
@mongodb-php-bot mongodb-php-bot mentioned this pull request Feb 10, 2025
Merged
@GromNaN GromNaN deleted the PHPLIB-1597 branch February 10, 2025 15:42
GromNaN added a commit to GromNaN/mongo-php-library that referenced this pull request Feb 23, 2026
@GromNaN
PHPLIB-1597 Accept a $-prefixed string anywhere an expression is acce…
dff4217 
…pted (mongodb#1571)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jmikola jmikola jmikola approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@GromNaN @jmikola